Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Count only for the first hit for subject or tissue within filename #173

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Count only for the first hit for subject or tissue within filename #173

wants to merge 2 commits into from

Conversation

yarikoptic
Copy link
Member

Also added assertion so we do not count incorrectly. But may be should be just a warning?

Closes #172

@yarikoptic yarikoptic requested a review from satra April 6, 2023 18:16
@satra
Copy link
Member

satra commented Apr 6, 2023

i think this also requires line 302 replaced with the sanitize function from dandi-cli.

@codecov
Copy link

codecov bot commented Apr 6, 2023 •
edited
Loading

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (b310e3e) 97.71% compared to head (d22ac9c) 97.72%.

Additional details and impacted files 
@@           Coverage Diff           @@
##           master     #173   +/-   ##
=======================================
  Coverage   97.71%   97.72%           
=======================================
  Files          17       17           
  Lines        1751     1755    +4     
=======================================
+ Hits         1711     1715    +4     
  Misses         40       40
Flag Coverage Δ
unittests 97.72% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
dandischema/metadata.py 96.69% <100.00%> (+0.06%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@yarikoptic
Copy link
Member Author

i think this also requires line 302 replaced with the sanitize function from dandi-cli.

did you try that and it resolved #172? would better be done in a separate PR then since this one does resolve issue too

@yarikoptic
Count only for the first hit for subject or tissue within filename
ed02dec 
Also added assertion so we do not count incorrectly. But may be should
be just a warning?

Closes #172
@yarikoptic
Copy link
Member Author

yarikoptic commented Apr 6, 2023 •
edited
Loading

i think this also requires line 302 replaced with the sanitize function from dandi-cli.

did you try that and it resolved #172? would better be done in a separate PR then since this one does resolve issue too

I tested, it does. Will submit a complimentary PR.

@yarikoptic
Copy link
Member Author

yarikoptic commented Apr 17, 2023 •
edited
Loading

@satra so what do you think about this one? It is complimentary to #175

satra
satra reviewed Apr 18, 2023
View reviewed changes
dandischema/metadata.py Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@satra satra satra left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Incorrect summaries (at least number of subjects) in some dandisets
2 participants
@yarikoptic @satra